Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: Fix compatibility with php 8.4 and newer ImageMagick library versions #704

Closed
wants to merge 4 commits into from
Closed

fix: Fix compatibility with php 8.4 and newer ImageMagick library versions #704

wants to merge 4 commits into from

Conversation

michalananapps
Copy link

No description provided.

@michalananapps michalananapps mentioned this pull request Nov 30, 2024
Closed
@calebdw
Copy link

calebdw commented Dec 5, 2024

@michalananapps,

Not sure if this is related to this PR or not, but I'm unable to install the latest imagick extension (with IM 6) in a PHP8.4 docker container:

#12 24.28 In /tmp/pear/temp/imagick/Imagick.stub.php:
#12 24.28 Imagick::newImage(): Parameter format has null default, but is not nullable
#12 24.29 make: *** [Makefile:191: /tmp/pear/temp/imagick/Imagick_arginfo.h] Error 1
#12 24.31 ERROR: `make INSTALL_ROOT="/tmp/pear/temp/pear-build-defaultuser8bmcdts2h2djeKWEFxQ/install-imagick-3.7.0" install' failed
#12 ERROR: process "/bin/sh -c pecl install imagick imap uv xdebug     && pecl install -o -f redis     && rm -rf /tmp/pear     && docker-php-ext-enable imagick imap redis" did not complete successfully: exit code: 1

because this is not explicitly nullable:
https://github.com/Imagick/imagick/blob/master/Imagick.stub.php#L1176

@michalananapps
Copy link
Author

Not sure if this is related to this PR or not, but I'm unable to install the latest imagick extension (with IM 6) in a PHP8.4 docker container:

This PR is one of the fixes for compatibility with PHP 8.4, but it is not directly related to your issue. Your issue is related to the Imagick.stub.php file, which was fixed in the develop branch six months ago but has not yet been included in an official release. You are currently using the PECL official release, where this fix is not available yet. Commit with the fix: Commit Link.

You can install any development version of Imagick using mlocati/php-extension-installer. For example:

COPY --from=mlocati/php-extension-installer /usr/bin/install-php-extensions /usr/local/bin/  
RUN install-php-extensions https://api.github.com/repos/michalananapps/imagick/tarball/cd0bc8f4269ee3845b2cc216f5555dc2a676712b

This command will install the version from the specified commit. However, you must ensure that you only install it from trusted sources. The example above uses the current commit of this PR's source branch. Use it at your own risk. 😊

@calebdw
Copy link

calebdw commented Dec 5, 2024

Awesome, thanks for the info---I didn't know this tool existed!

It looks like PHP 8.4 support for imagick is built into the tool so you can just use:

ADD --chmod=0755 https://github.com/mlocati/docker-php-extension-installer/releases/latest/download/install-php-extensions /usr/local/bin/
RUN install-php-extensions imagick

@Inscure
Copy link

Inscure commented Jan 30, 2025

@Danack could you look at this PR? thank you

bukka
bukka requested changes Feb 28, 2025
View reviewed changes
Copy link
Collaborator

@bukka bukka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please split the CI (including util/calculate_cflags.php) and version check to two PR's? Please also remove the imagick.c changes as they are already merged. They are independent.

imagick.c
@@ -610,7 +610,7 @@ static zval *php_imagick_read_property(zend_object *object, zend_string *member,
if (format) {
retval = rv;
ZVAL_STRING(retval, format);
php_strtolower(Z_STRVAL_P(retval), Z_STRLEN_P(retval));
zend_str_tolower(Z_STRVAL_P(retval), Z_STRLEN_P(retval));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those replacements are already merged to develop so please remove them.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bukka First of all, I wanted to say that I'm really happy that you took care of this project :). As for the split, I can do it, but some of the changes related to CI require modifications that are part of the version check. I'll first submit a PR with the version check and then another one for CI with the newer PHP versions.

Copy link
Collaborator

@bukka bukka Mar 1, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool thanks. Or if it's easier, you can just split it to two commits and push force it here...

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also please rebase on the latest develop - I pushed few commits to fix build.

Copy link
Author

@michalananapps michalananapps Mar 1, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bukka is there a plan to drop PHP5.4 and PHP5.5 support?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bukka 1st part: #716

We can close this (#704).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bukka is there a plan to drop PHP5.4 and PHP5.5 support?

Yeah I want to actually drop them and keep just 5.6 for the next release. Then I would like to drop PHP 5 support completely in the following release.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

very nice to hear you guys getting Imagick into shape again! But honestly, why not drop any EOL version, anything below PHP 8.1?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are long term support commercial plans and some distros keep it longer so currently the oldest supported version that I'm aware of is 7.2 but think 7.0 and 7.1 don't really cost much extra in terms of maintanence so I don't really mind to keep those if they just work in pipeline. The 5.6 got some updates and dropping the support should be also done with clean up which would delay the next release so that's why I want to do it in the follow up version.

@bukka
Copy link
Collaborator

bukka commented Mar 2, 2025

#716 is merged so closing this

@bukka bukka closed this Mar 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bukka bukka bukka requested changes

@onlime onlime onlime left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@michalananapps @calebdw @Inscure @bukka @onlime